Skip to content

Conversation

@newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Apr 10, 2025

Changelog

- description: |
    Make the output format flag for the `query utxo` command only have one default: JSON (rather than a different default depending on whether the output file is specified)
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

A new way to specify flags in code that makes the default flag specification more concise and DRY whilst making it easier to standardise on flag help wording whilst still providing flexibility.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

, f.format
, if f.options.isDefault then " (default)" else ""
, "."
]
Copy link
Contributor Author

@newhoggy newhoggy Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still have flexibility on the help for the flag by providing a rendering function like this.

parserFromFlags offers that flexibility and parserFromFormatFlags takes that flexibility away so we can be consistent.

@newhoggy newhoggy force-pushed the newhoggy/new-way-to-specify-flags branch from 8c202b9 to 3ab349e Compare April 10, 2025 02:46
@newhoggy newhoggy force-pushed the newhoggy/new-way-to-specify-flags branch from 3ab349e to db9f485 Compare April 10, 2025 02:52
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the overall changes but OverloadedRecordDot & NoFieldSelectors need to be removed: #1133 (comment) and #1133 (comment)

@newhoggy newhoggy force-pushed the newhoggy/new-way-to-specify-flags branch 2 times, most recently from e8cef26 to 0df0d3a Compare April 11, 2025 22:40
@newhoggy newhoggy force-pushed the newhoggy/new-way-to-specify-flags branch from 0df0d3a to 3fbc48c Compare April 11, 2025 22:46
@newhoggy newhoggy dismissed stale reviews from Jimbo4350 and carbolymer April 11, 2025 22:47

Comments addressed

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@newhoggy newhoggy requested a review from Jimbo4350 April 14, 2025 20:10
@newhoggy newhoggy dismissed Jimbo4350’s stale review April 14, 2025 20:12

Comments addressed

import Options.Applicative (Parser)
import Options.Applicative qualified as Opt

-- | Create a parser from a help rendering function and list of flags.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice haddock 👍

@newhoggy newhoggy force-pushed the newhoggy/new-way-to-specify-flags branch from 6538092 to 9e9f0bb Compare April 16, 2025 18:17
@newhoggy newhoggy requested a review from carbolymer April 16, 2025 18:30
@newhoggy newhoggy force-pushed the newhoggy/new-way-to-specify-flags branch from 9e9f0bb to c5bd903 Compare April 16, 2025 18:35
@newhoggy newhoggy dismissed carbolymer’s stale review April 16, 2025 18:36

Comments addressed

@newhoggy newhoggy added this pull request to the merge queue Apr 16, 2025
Merged via the queue into master with commit 8413e1a Apr 16, 2025
25 checks passed
@newhoggy newhoggy deleted the newhoggy/new-way-to-specify-flags branch April 16, 2025 19:23
@newhoggy
Copy link
Contributor Author

Addresses #566

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants